Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add internal docs and do other improvements #80

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

stIncMale
Copy link
Member

@stIncMale stIncMale commented Mar 26, 2025

This PR is a follow-up for #69 (comment).

  • We don't need to document the thread-safety requirement of MongoDialect, because it is documented on org.hibernate.dialect.Dialect.
  • We don't need to document that SqlAstTranslators created by MongoTranslatorFactory are single-use, because it is documented on org.hibernate.sql.ast.SqlAstTranslatorFactory.

@stIncMale stIncMale self-assigned this Mar 26, 2025
@@ -46,6 +46,6 @@ protected DatabaseVersion getMinimumSupportedVersion() {

@Override
public SqlAstTranslatorFactory getSqlAstTranslatorFactory() {
return new MongoTranslatorFactory();
return MongoTranslatorFactory.INSTANCE;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way we work around the issue fixed in hibernate/hibernate-orm#9900, because we are not getting that fix in Hibernate ORM 6.x.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your raising the issue and it is a quick fix. On the other hand, it seems trivial to me for we only avoid one unnecessary duplicated object creation (which usually is not a big deal for the creation is not costly).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other id generator defect is more important and hard to fix from Hibernate side. I created a PR at hibernate/hibernate-orm#9965. Let us see.

* The instance methods like {@link #getContributorName()}, {@link #contribute(AdditionalMappingContributions,
* InFlightMetadataCollector, ResourceStreamLocator, MetadataBuildingContext)} are called multiple times if multiple
* {@link Metadata} instances are {@linkplain MetadataSources#buildMetadata() built} using the same
* {@link BootstrapServiceRegistry}.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This Javadoc is visible to public even though it is within internal package, right?

To my best knowledge, I don't expect any Hibernate contributor methods would be called more than once in one SessionFactory. So the above doc and similar ones in this PR really confuse me.

I'd thought you plan to put down such doc outside our code. Putting it here seems to hurt our product for they might clarify some doubts to us but would confuse others (including me).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This Javadoc is visible to public even though it is within internal package, right?

Yes, our whole codebase is publicly visible.

To my best knowledge, I don't expect any Hibernate contributor methods would be called more than once in one SessionFactory.

SessionFactory has nothing to do with the invocations of MongoAdditionalMappingContributor.contribute. This method is called neither by SessionFactory, nor even when a SessionFactory instance is built by the SessionFactoryBuilder.build method.

Putting it here seems to hurt our product for they might clarify some doubts to us but would confuse others (including me).

  • This is not our public API documentation, because the documented class is not part of our API. Thus, it does not apply to "others".
  • What do you mean exactly by "confuse"? (Does the proposed documentation say something that is false? Does it make one to believe something that is false because it is ill-formulated?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we need to build multiple Metadata? Yeah, Hibernate doesn't disallow, but I dont' understand why we need to consider this case. From my best knowledge, such contributor only needs to be called once.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not our public API documentation, because the documented class is not part of our API. Thus, it does not apply to "others".

Given our codebase is public, there is still possibility that the doc could be read by others (e.g. community contributor or Hibernate developer's browsing out of curiosity). If the project is private, I would have no such concern.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean exactly by "confuse"? (Does the proposed documentation say something that is false? Does it make one to believe something that is false because it is ill-formulated?)

Let me elaborate on why I got confused.

Hibernate's contributor is to be created by default constructor (via JDK's ServiceLoader mechanism or ClassLoaderService in Hibernate), so it is a typical one-use object in the sense that once it is loaded (or created), its contribute() method is invoked and then that is it and the contributor object is subject to garbage collecting. Next time a new instance will be created and loaded, but I don't think it will be reused more than once.

Such contributor hooking point is hard-coded in Hibernate code as below:


		final Collection<AdditionalMappingContributor> additionalMappingContributors = classLoaderService.loadJavaServices( AdditionalMappingContributor.class );
		additionalMappingContributors.forEach( (contributor) -> {
			contributions.setCurrentContributor( contributor.getContributorName() );
			try {
				contributor.contribute(
						contributions,
						metadataCollector,
						classLoaderService,
						rootMetadataBuildingContext
				);
			}
			finally {
				contributions.setCurrentContributor( null );
			}
		} );

As you can see, for each contributor, its two methods will be called only once after it is loaded or created. So it seems a typical one-use scenario as our various mql translators.

* The instance methods like {@link #contribute(StandardServiceRegistryBuilder)} are called multiple times if
* multiple {@link StandardServiceRegistry} instances are {@linkplain StandardServiceRegistryBuilder#build() built}
* using the same {@link BootstrapServiceRegistry}.
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, I failed to see why multiple StandardServiceRegistry need to be build from one BootstrapServiceRegistry. Could we move the confusing doc out of the code base?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that matters. What does matter is that doing so is possible with the API of Hibernate ORM, and it is not forbidden by the API documentation, so we must take that into account when we are extending Hibernate ORM.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is why I got confused. Yeah, it is not forbidden, but why we need to cover it? I never know of such scenario that we need to create multiple StandardServiceRegistry, which seems to invite trouble.

Copy link
Contributor

@NathanQingyangXu NathanQingyangXu Apr 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So from my understanding, Hibernate's SessionFactory is pretty heavy-weighted (it is super slow for it tries to prepare everything so runtime perf could be as fast as possible; one example is it tries to finish static SQL translations as much as possible so they could be reused). For that reason, for typical usage, it is seldom created multiple times (in typical Spring JPA usage, one single SessionFactory is bound to the global Spring Bean Context), unless there are good reasons.

If multiple SessionFactory is justified (maybe each one uses a subset of the META-INF/services so they could share the same jar library without conflict; then it requires customized BootstrapServiceRegistry, one of whose components is the ClassLoaderService which could be customized), different BootstrapServiceRegistry is expected to be created for each of them. Different BootstrapServiceRegistry leads to different StandardServiceRegistry (as explained above, maybe due to different class loaders), otherwise why create different SessionFactory in the first place?

During runtime, one SessionFactory has one single ServiceRegistry and its default implementation is StandardServiceRegistry as its API below (in org.hibernate.engine.spi.SessionFactoryImplementor):

ServiceRegistryImplementor getServiceRegistry();

From my understanding, one and only one StandardServiceRegistry is needed, and StandardServiceRegistryBuilder is the de-factor one-time stuff. When it has finished building, it seems only in theory that it could be invoked again (why? one StandardServiceRegistry is not enough?). That is what puzzles me. Yeah, it is possible but why emphasize such fact in Javadoc (arguably not internal doc for it is likely that community contributor needs to read it to figure out something).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above comment also explains why I got puzzled by your previous PR; but you modified it to make it aligned with reality so I approved that.

Copy link
Contributor

@NathanQingyangXu NathanQingyangXu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest closing this PR for it only ends up with confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants